Support zinc invalidation for type arguments in macro calls #23900
Conversation
|
Hi @bishabosha, would you be able to review this? If not, then I think I can ask @tgodzik, but he's more of an expert on the zinc side than the compiler side I think. |
| private def addMacroDependency(sym: Symbol)(using Context): Unit = | ||
| if (allowsDependencyByMacroExpansion) { | ||
| if (!ignoreDependency(sym)) { | ||
| if (!sym.is(Package)) { |
There was a problem hiding this comment.
Isn't the package test redundant, because it's already in TypeDependencyTraverser.traverse?
| val traverser = new TypeDependencyTraverser { | ||
| def addDependency(symbol: Symbol) = addMacroDependency(symbol) | ||
| } | ||
| trees.foreach(tree => traverser.traverse(tree.tpe)) |
There was a problem hiding this comment.
Looks like this whole function should be elided when the macro dependency tracking is unavailable
jchyb
force-pushed
the
fix-23852-invalidate-macro-type-arg
branch
from
d420e21 to
6fc1afb
Compare
| // as it was added later to the zinc.apiinfo DependencyContext enum | ||
| // e.g. pre 1.10.x sbt would throw java.lang.NoSuchFieldError errors here | ||
| lazy val allowsDependencyByMacroExpansion = | ||
| classOf[DependencyContext].getFields().exists(_.getName() == "DependencyByMacroExpansion") |
There was a problem hiding this comment.
I think this is a mistake, this is using the static type (meaning field names are statically available), so this will always pass. (e.g. you made Zinc 1.10.x a library dependency)
There should be a way to dynamically test actual linked zinc version
There was a problem hiding this comment.
I think getFields() still uses reflection (https://docs.oracle.com/javase/8/docs/api/java/lang/Class.html#getFields--), at the very least the check still works when I publish locally and use older sbt versions relying on older zinc (as opposed to what would happen without that check, where it would crash).
I did find just now that zinc ships with incrementalcompiler.version.properties resource file (but no utils for that, unfortunately), so we could probably read that if you prefer (also thank you for the timely reviews! and apologies for taking this long to update the PR).
There was a problem hiding this comment.
I've tested just now and something like this:
lazy val allowsDependencyByMacroExpansion =
val reader = new BufferedReader(
new InputStreamReader(getClass().getResourceAsStream("/incrementalcompiler.version.properties"), java.nio.charset.StandardCharsets.UTF_8)
)
val VersionRegex = "^version\\=(\\d+)\\.(\\d+)\\..*".r
reader.lines.toList.asScala.exists {
case VersionRegex(major, minor) if major.toInt > 1 || (major.toInt == 1 && minor.toInt >= 10) => true
case _ => false
}also works. Let me know if I should change it to that
3 participants
Fixes #23852
Closes #19426
Around a year ago a new
DependencyContextvalue was added to the zinc api:DependencyByMacroExpansion.It causes the recorded dependency to trigger recompilation if there is any API change in the recorded type, not just if the feudal/method member that was explicitly referenced was changed. So now, for every inline call:
macroCall[T1, T2, ...], in filecall.scala, if anything changes in a TN definition,call.scalais recompiled.Since
DependencyByMacroExpansionwas added later to the API, referencing it in older versions (e.g. any sbt <1.10.0), would cause crashes. to avoid that, we use reflection to check if that field exists.These changes do not seem to affect #23783, likely the way annotations are handled needs to be changed in zinc itself, as the APIInfo phase does successfully record the change in the annotation argument.
Related discussion: sbt/zinc#1316 (a PR adding the functionality to zinc and scala-2 plugin).